Skip to content

fix: store all peers in storage #420

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
xdustinface merged 2 commits into from
Feb 9, 2026
Merged

fix: store all peers in storage #420

xdustinface merged 2 commits into from
Feb 9, 2026
+39 −19

Conversation

@xdustinface
Copy link
Collaborator

@xdustinface xdustinface commented Feb 6, 2026 •
edited
Loading

We currently fetch peers to store via get_addresses_for_peer(MAX_ADDR_TO_STORE) which caps the number internally to MAX_ADDR_TO_SEND. Its not a problem really right now since we so far not even close to end up with so many peers because peer discovery isn't functional but its still wrong.

  • First commit refactors get_known_addresses to return the full AddrV2Message instead of just the SocketAddr.
  • Second commit fixes it by using get_known_addresses for peer saving.

Working towards closing:

Summary by CodeRabbit

Release Notes

  • Refactor

    • Updated internal peer address handling and storage mechanisms throughout the networking layer.
    • Enhanced peer selection logic to work with improved address representations.

  • Tests

    • Added test utilities for improved peer address testing.
    • Updated test infrastructure to support the refactored networking components.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 6, 2026 •
edited
Loading

📝 Walkthrough

Walkthrough

The pull request modifies peer address handling throughout the network module, changing the internal API to work with AddrV2Message objects instead of raw SocketAddr types. This affects address retrieval, peer selection algorithms, and test infrastructure while maintaining the same underlying functionality.

Changes

Cohort / File(s) Summary
Core API Updates
dash-spv/src/network/addrv2.rs, dash-spv/src/network/reputation.rs		 Method signatures updated to accept and return Vec<AddrV2Message> instead of Vec<SocketAddr>. get_known_addresses() now returns full AddrV2Message objects; select_best_peers() accepts AddrV2Message list with internal conversion to SocketAddr for reputation scoring.
Implementation Changes
dash-spv/src/network/manager.rs		 Updated to call get_known_addresses() for persisting known addresses to disk instead of the previous limited subset approach.
Test Infrastructure
dash-spv/src/network/reputation_tests.rs, dash-spv/tests/peer_test.rs		 Test setup and assertions updated to construct AddrV2Message objects and extract underlying SocketAddr values via socket_addr().unwrap() for comparisons.
Test Utilities
dash/src/test_utils/mod.rs, dash/src/test_utils/network.rs		 Added network module with new AddrV2Message::dummy() helper function to simplify construction of test message instances with configurable time, IPv4 address, and port.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

Across the network hops our furry friend,
AddrV2Messages 'round each bend,
Type-safe addresses dance and play,
Testing dummies light the way! 🐰✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: store all peers in storage' directly and clearly describes the main change: fixing the peer storage mechanism to persist all known peers instead of a capped subset.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/store-all-peers

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@xdustinface xdustinface marked this pull request as ready for review February 6, 2026 16:27
coderabbitai[bot]
coderabbitai bot reviewed Feb 6, 2026
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
dash-spv/src/network/manager.rs (1)

1252-1258: ⚠️ Potential issue | 🟠 Major

shutdown() still uses get_addresses_for_peer(MAX_ADDR_TO_STORE) — same cap bug persists here.

The maintenance loop (line 889) was fixed to use get_known_addresses(), but shutdown() still calls get_addresses_for_peer(MAX_ADDR_TO_STORE) which internally caps the result to MAX_ADDR_TO_SEND (line 102 in addrv2.rs). Since shutdown is the last chance to persist peers, this is arguably the most important call site to fix.

Proposed fix 
-        let addresses = self.addrv2_handler.get_addresses_for_peer(MAX_ADDR_TO_STORE).await;
+        let addresses = self.addrv2_handler.get_known_addresses().await;

@xdustinface xdustinface mentioned this pull request Feb 6, 2026
Draft
@xdustinface xdustinface merged commit 17ed152 into v0.42-dev Feb 9, 2026
53 checks passed
@xdustinface xdustinface deleted the fix/store-all-peers branch February 9, 2026 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@ZocoLini ZocoLini ZocoLini approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@xdustinface @ZocoLini